SF-3875 Improve note sync performance - #4003
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4003 +/- ##
=======================================
Coverage 81.06% 81.07%
=======================================
Files 660 661 +1
Lines 43036 43057 +21
Branches 7057 7057
=======================================
+ Hits 34887 34908 +21
Misses 6977 6977
Partials 1172 1172 ☔ View full report in Codecov by Harness. |
pmachapman
left a comment
There was a problem hiding this comment.
@Nateowami Has this bug been reported to the Paratext team? I can't actually think of a way of fixing the bug within Comment.CompareTo(), as it seems a grouping (which your code does) is needed to fix the sorting.
@pmachapman reviewed 4 files and all commit messages, and made 2 comments.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on Nateowami).
src/SIL.XForge.Scripture/Services/CommentManagerExtensions.cs line 41 at r1 (raw file):
threads.Add(thread); } }
We could speed this up by iterating over manager.AllComments, then building the CommentThreads from there. In particular, we would group then sort the comments correctly via our own version of the logic in CommentManager.SortAndGroupThreads.
I'm not making this blocking, as I am interested in your thoughts, although perhaps I lean towards us implementing our own iterator as the PT CommentManager.FindThreads() code has not changed in years?
Code quote:
foreach (CommentThread thread in manager.FindThreads())
{
if (threadIndexById.TryGetValue(thread.Id, out int index))
threads[index] = manager.FindThread(thread.Id);
else
{
threadIndexById[thread.Id] = threads.Count;
threads.Add(thread);
}
}192df5a to
f244bc6
Compare
Nateowami
left a comment
There was a problem hiding this comment.
@Nateowami made 1 comment and resolved 1 discussion.
Reviewable status: 2 of 4 files reviewed, all discussions resolved (waiting on pmachapman).
src/SIL.XForge.Scripture/Services/CommentManagerExtensions.cs line 41 at r1 (raw file):
Previously, pmachapman (Peter Chapman) wrote…
We could speed this up by iterating over
manager.AllComments, then building theCommentThreadsfrom there. In particular, we would group then sort the comments correctly via our own version of the logic inCommentManager.SortAndGroupThreads.I'm not making this blocking, as I am interested in your thoughts, although perhaps I lean towards us implementing our own iterator as the PT
CommentManager.FindThreads()code has not changed in years?
Done.
f244bc6 to
0fb6f37
Compare
- Performance improvement comes by looking up all note threads in a single pass and placing them in a dictionary - Also fixed a bug caused by CommentManager.FindThreads not properly grouping threads with comments on multiple verses.
0fb6f37 to
9e12c21
Compare
pmachapman
left a comment
There was a problem hiding this comment.
@pmachapman reviewed 2 files and all commit messages, and made 1 comment.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on Nateowami).
CommentManager.FindThreadsnot properly grouping threads with comments on multiple versesProjects with tons of threads take a long time to sync, and I think it grows with
[thread count] * [comments count across all threads]. This results in some very slow syncs that prevent other projects from being able to sync, since we only allow one sync at a time (the worst offender on live spent 17min 18s just on note threads).The bigger change on the PR is wrapping
CommentManager.FindThreadsto handle cases where it emits the same thread ID twice, along with a test to demonstrate a) that it does this with certain threads, and b) that the wrapper handles that scenario.This change is